Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FIFO handling on Solaris #70

Merged
merged 3 commits into from
Nov 20, 2022
Merged

Fix FIFO handling on Solaris #70

merged 3 commits into from
Nov 20, 2022

Conversation

greatroar
Copy link
Contributor

On Solaris, files need to be opened to obtain or change their attributes. But opening a FIFO file causes the calling thread to block until the write end is opened. Files are now opened using O_NONBLOCK to prevent this from happening.

Should fix restic/restic#4003.

Also fix comment on stringsFromByteSlice.

This code compiles, but I've not been able to test it without access to a Solaris box. I'm hoping @andy-js, @gco, and/or @racingmars can review/test this.

@racingmars
Copy link

racingmars commented Nov 5, 2022

Starting with today's restic master branch (restic/restic@24a2e5c), I updated go.mod to include the line:

replace github.com/pkg/xattr => github.com/greatroar/xattr solaris-fifo

After building restic with the updated xattr package, restic is able to successfully backup a directory containing a FIFO file:

mkdir repo target
mkfifo target/myfifo
restic -r repo init
restic -r repo backup target

That said... as far as I can tell, neither restic 0.14.0 release nor this updated build are actually backing up xattrs on regular files, but since the behavior is the same in the release version I'm guessing that has nothing to do with this change. (Although I see there is still an open issue in restic about not backing up ACLs on SmartOS, for example, so this appears to be a known issue.)

@gco
Copy link
Contributor

gco commented Nov 6, 2022

Most of the changes aren't directly relevant to this issue, only the O_NONBLOCK is necessary. Reverting back many of the changes from #62 concerns me due to the file descriptor lifetime issues addressed there.

Due to errors with doors and whatnot, my local copy of xattr currently has a function called to check if a file can have xattrs and is called from the various entry points into xattr:

func xattr_supported(path string) (bool, error) {
        var st = unix.Stat_t{}
        sterr := unix.Lstat(path, &st)
        if sterr != nil {
                return false, sterr
        }
        if ((st.Mode & unix.S_IFMT) != unix.S_IFREG) &&
            ((st.Mode & unix.S_IFMT) != unix.S_IFDIR) {
                return false, nil
        }
        return true, nil
}

Which isn't pretty and adds an extra system call for every path but avoids problems backing up parts of the root filesystem without having to add files to the exclude list. There are still filesystems like tmpfs which don't support xattrs at all, but you get unix.EINVAL there which gets changed to unix.ENOTSUP for the consumers of xattr.

On Solaris, files need to be opened to obtain or change their
attributes. But opening a FIFO file causes the calling thread to block
until the write end is opened. Files are now opened using O_NONBLOCK to
prevent this from happening.

Should fix restic/restic#4003.

Also fix comment on stringsFromByteSlice.
@greatroar
Copy link
Contributor Author

greatroar commented Nov 6, 2022

Oh, I wasn't aware that that's what the NewFile calls were for. I just wasn't sure whether I could actually pass O_NONBLOCK to os.OpenFile. Pushed a smaller patch that does that instead. Judging from how os.O_* is implemented on Solaris, this should work, though it's a bit of a hack to combine os.O_* and unix.O_* constants.

@kuba--
Copy link
Member

kuba-- commented Nov 6, 2022

Thank you guys - great work!
Unfortunately, I do not have access to Solaris, right now. So, if any Solaris' user can confirm that this PR works/fixes the issue, I'll be happy to merge it

@gco
Copy link
Contributor

gco commented Nov 7, 2022

@greatroar I have the same hack in the copy I've been using but you're right--- it doesn't seem legal.

Passing this flag to os.Open works, but this relies on undocumented
implementation details of package os.
@greatroar
Copy link
Contributor Author

Pushed a version that opens with unix.Open but uses os.NewFile on the resulting fds.

@andy-js
Copy link
Contributor

andy-js commented Nov 9, 2022

This looks good to me. I will see about testing.

@gco
Copy link
Contributor

gco commented Nov 10, 2022

xattr.Remove hangs with your built-in tests, see patch below. Also, the tests hang when I remove the O_NONBLOCK from xattr_solaris.go (as one would expect). I tried adding timer = time.AfterFunc(...) before the test and timer.Stop() after to catch the hang.

diff --git a/xattr_solaris.go b/xattr_solaris.go
index 57afaee..ce8b98a 100644
--- a/xattr_solaris.go
+++ b/xattr_solaris.go
@@ -87,7 +87,7 @@ func fsetxattr(f *os.File, name string, data []byte, flags int) error {
 }
 
 func removexattr(path string, name string) error {
-       fd, err := unix.Open(path, unix.O_RDONLY|unix.O_XATTR, 0)
+       fd, err := unix.Open(path, unix.O_RDONLY|unix.O_XATTR|unix.O_NONBLOCK, 0)
        if err != nil {
                return err
        }

@gco
Copy link
Contributor

gco commented Nov 10, 2022

This ensures the test runs in a bounded amount of time:

diff --git a/xattr_test.go b/xattr_test.go
index 2088288..36a3124 100644
--- a/xattr_test.go
+++ b/xattr_test.go
@@ -12,6 +12,7 @@ import (
        "runtime"
        "syscall"
        "testing"
+       "time"
 
        "golang.org/x/sys/unix"
 )
@@ -280,11 +281,15 @@ func TestFIFO(t *testing.T) {
                t.Fatal(err)
        }
 
+       timer := time.AfterFunc(30*time.Second, func() { panic("TestFIFO hang") })
+
        // We only care about this not blocking.
        _ = Set(fifo, "foo", []byte("bar"))
        _, _ = Get(fifo, "foo")
        _, _ = List(fifo)
        _ = Remove(fifo, "foo")
+
+       timer.Stop()
 }

@greatroar
Copy link
Contributor Author

greatroar commented Nov 11, 2022

Ok, added O_NONBLOCK in removexattr, and O_CLOEXEC while I was at it. I'd rather leave timeouts to the go test command.

@kuba--
Copy link
Member

kuba-- commented Nov 17, 2022

Did anyone verify and also can approve the PR (so, I can be sure it does not break anything and I can merge it)?
Thanks!

@kuba-- kuba-- merged commit 35026bb into pkg:master Nov 20, 2022
tomponline referenced this pull request in canonical/lxd Jul 22, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/pkg/xattr](https://togithub.com/pkg/xattr) | `v0.4.9` ->
`v0.4.10` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fpkg%2fxattr/v0.4.10?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fpkg%2fxattr/v0.4.10?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fpkg%2fxattr/v0.4.9/v0.4.10?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fpkg%2fxattr/v0.4.9/v0.4.10?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>pkg/xattr (github.com/pkg/xattr)</summary>

### [`v0.4.10`](https://togithub.com/pkg/xattr/releases/tag/v0.4.10)

[Compare
Source](https://togithub.com/pkg/xattr/compare/v0.4.9...v0.4.10)

#### What's Changed

- Fix FIFO handling on Solaris by
[@&#8203;greatroar](https://togithub.com/greatroar) in
[https://github.com/pkg/xattr/pull/70](https://togithub.com/pkg/xattr/pull/70)
- Add support for go 1.21 by
[@&#8203;kuba--](https://togithub.com/kuba--) in
[https://github.com/pkg/xattr/pull/72](https://togithub.com/pkg/xattr/pull/72)
- Detect the need to increase buffer size on TrueNAS SCALE. by
[@&#8203;erikrose](https://togithub.com/erikrose) in
[https://github.com/pkg/xattr/pull/73](https://togithub.com/pkg/xattr/pull/73)

#### New Contributors

- [@&#8203;erikrose](https://togithub.com/erikrose) made their first
contribution in
[https://github.com/pkg/xattr/pull/73](https://togithub.com/pkg/xattr/pull/73)

**Full Changelog**:
pkg/xattr@v0.4.9...v0.4.10

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on Monday" (UTC),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/canonical/lxd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MzguMCIsInVwZGF0ZWRJblZlciI6IjM3LjQzOC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants